Add test for calculate_index function #495

TinyMarsh · 2024-08-15T19:55:46Z

Turns out that I thought it would be worth creating a separate PR for implementing this test as I've decided to be more thorough with things.

While creating the test fixtures I noticed that there existed some code in Simulation.Test.cpp to help create the fixtures I was using in AnalysisModule.Test.cpp. I have moved much of this code to simulation.h/.cpp and have both test files include this header.

Closes #470

for more information, see https://pre-commit.ci

github-actions

clang-tidy made some suggestions

src/HealthGPS.Tests/AnalysisModule.Test.cpp

for more information, see https://pre-commit.ci

github-actions

clang-tidy made some suggestions

src/HealthGPS.Tests/AnalysisModule.Test.cpp

src/HealthGPS.Tests/simulation.h

for more information, see https://pre-commit.ci

github-actions

clang-tidy made some suggestions

src/HealthGPS.Tests/AnalysisModule.Test.cpp

src/HealthGPS.Tests/simulation.h

for more information, see https://pre-commit.ci

TinyMarsh · 2024-09-05T12:30:29Z

Hey @jamesturner246 , @alexdewar. This is ready for a review now, when you have time. The Windows build is failing for reasons I can't quite get to the bottom of, but I'll have more of a poke around and see if I can fixt that.

alexdewar · 2024-09-05T12:42:16Z

@TinyMarsh The Windows build is failing because of a missing #include. It must have changed in MSVC's standard lib...

I had the same problem on my branch. Fix here:

git cherry-pick 3481ff4958e3b838a9062ef8a76d213f666555fb

alexdewar

Looks good on the whole -- and the test module seems useful. There are a couple of unused functions you've added though and I think we could do with a few more test cases for calculate_index.

src/HealthGPS.Tests/AnalysisModule.Test.cpp

src/HealthGPS.Tests/RiskFactorData.cpp

src/HealthGPS/analysis_module.cpp

alexdewar · 2024-09-05T13:00:50Z

src/HealthGPS.Tests/AnalysisModule.Test.cpp

+
+    size_t index_1 = analysis_module->calculate_index(test_person_1);
+
+    ASSERT_EQ(index_1, 29);


Do we want to test some more indexes here? E.g. you could check it works with 1, 2 and 3 dimensional matrices

TinyMarsh · 2024-09-05T13:47:14Z

Thanks @alexdewar. Yes absolutely we could probably do with more test cases. I meant to do that but I think I just got excited that I actually managed to get a test running and rushed out a PR 😄. I will add some more cases and clean up the code per your other comments.

jamesturner246

Very good! Much has already been stated, just a couple minor things and a question.

As @alexdewar says, maybe a couple more tests/cases in this PR and looks god to go!

src/HealthGPS/analysis_module.h

src/HealthGPS.Tests/simulation.h

src/HealthGPS.Tests/simulation.cpp

src/HealthGPS.Tests/CMakeLists.txt

src/HealthGPS.Tests/AnalysisModule.Test.cpp

codecov · 2024-10-07T15:33:46Z

Codecov Report

Attention: Patch coverage is 97.05882% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (extend_analysis@52e23d9). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/HealthGPS/analysis_module.cpp	83.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             extend_analysis     #495   +/-   ##
==================================================
  Coverage                   ?   49.92%           
==================================================
  Files                      ?      160           
  Lines                      ?     7732           
  Branches                   ?     1027           
==================================================
  Hits                       ?     3860           
  Misses                     ?     3677           
  Partials                   ?      195

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for more information, see https://pre-commit.ci

TinyMarsh · 2024-10-09T14:22:07Z

Hi @alexdewar @jamesturner246 this should be ready for a re-review now. I have made some changes based on your feedback.

I noticed an error in the logic of the calculate_index function (this is why we do tests I suppose!) which was a big head-scratcher that ended up being a small adjustment. So I have fixed that and also increased the population age diversity and included an extra test to cover an extra age value.

The testing of varying dimensions will have to be done later (I will create an issue), as we currently have it hard-coded for 2 dimensions (age and gender in factors_to_calculate_).

alexdewar

There's some mysterious commented-out code, but other than that, this LGTM!

alexdewar · 2024-10-11T10:08:35Z

src/HealthGPS/analysis_module.cpp

-                                  std::ref(context), std::ref(result));
+    // auto handle = core::run_async(&AnalysisModule::calculate_historical_statistics, this,
+    //                               std::ref(context), std::ref(result));
+    // handle.get();


Is this not needed?

Yeah good spot. It shouldn't be commented out. This is left over from when I was trying to debug the test failing. With these two lines uncommented, I get the following test failure;

Start 236: TestAnalysisModule.CalculateIndex 236/236 Test #236: TestAnalysisModule.CalculateIndex ..............................***Failed 0.07 sec Initialising with a custom GTest main function. Using default test data store ... Test location..: /home/ryan/projects/healthgps/out/build/linux-debug/src/HealthGPS.Tests Test data store: /home/ryan/projects/healthgps/data Note: Google Test filter = TestAnalysisModule.CalculateIndex [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from TestAnalysisModule [ RUN ] TestAnalysisModule.CalculateIndex unknown file: Failure C++ exception with description "map::at" thrown in the test fixture's constructor. [ FAILED ] TestAnalysisModule.CalculateIndex (57 ms) [----------] 1 test from TestAnalysisModule (57 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test suite ran. (57 ms total) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] TestAnalysisModule.CalculateIndex 1 FAILED TEST 99% tests passed, 1 tests failed out of 236 Total Test time (real) = 15.35 sec The following tests FAILED: 236 - TestAnalysisModule.CalculateIndex (Failed) Errors while running CTest

I think this is because the RuntimeContext object that is created in the test fixture constructor is not properly initialised with the required map elements for some map that the commented-out handle.get() function above is trying to access.

Not quite there yet then. I feel like this is going to be a pain to sort out.

Fixed and wasn't too bad in the end. The RuntimeContext needed the current year to be set.

for more information, see https://pre-commit.ci

TinyMarsh and others added 2 commits August 15, 2024 20:54

Add test for calculate_index function

e4656e2

[pre-commit.ci] auto fixes from pre-commit.com hooks

dfc7e12

for more information, see https://pre-commit.ci

github-actions bot reviewed Aug 15, 2024

View reviewed changes

src/HealthGPS.Tests/AnalysisModule.Test.cpp Outdated Show resolved Hide resolved

TinyMarsh and others added 2 commits August 17, 2024 11:58

Move simulation logic to shared location

fa85412

[pre-commit.ci] auto fixes from pre-commit.com hooks

34d625a

for more information, see https://pre-commit.ci

github-actions bot reviewed Aug 17, 2024

View reviewed changes

src/HealthGPS.Tests/AnalysisModule.Test.cpp Outdated Show resolved Hide resolved

src/HealthGPS.Tests/AnalysisModule.Test.cpp Outdated Show resolved Hide resolved

src/HealthGPS.Tests/simulation.h Outdated Show resolved Hide resolved

TinyMarsh linked an issue Aug 27, 2024 that may be closed by this pull request

Add test for calculate_index function in the analysis module #470

Closed

TinyMarsh and others added 2 commits August 31, 2024 02:08

Amend test fixture and analysis_module code for running test

a7d8000

[pre-commit.ci] auto fixes from pre-commit.com hooks

85f23a9

for more information, see https://pre-commit.ci

github-actions bot reviewed Aug 31, 2024

View reviewed changes

src/HealthGPS.Tests/AnalysisModule.Test.cpp Outdated Show resolved Hide resolved

src/HealthGPS.Tests/AnalysisModule.Test.cpp Show resolved Hide resolved

src/HealthGPS.Tests/simulation.h Outdated Show resolved Hide resolved

TinyMarsh and others added 2 commits September 5, 2024 11:36

Make clang-tidy happy

9070c85

[pre-commit.ci] auto fixes from pre-commit.com hooks

97a8ca5

for more information, see https://pre-commit.ci

TinyMarsh requested review from jamesturner246 and alexdewar September 5, 2024 12:28

TinyMarsh marked this pull request as ready for review September 5, 2024 12:29

alexdewar requested changes Sep 5, 2024

View reviewed changes

Add missing #include

2ce17d8

jamesturner246 requested changes Sep 6, 2024

View reviewed changes

TinyMarsh added 3 commits September 6, 2024 15:13

Add necessary include

ed0bcba

Cast as uint to avoid warning

e8066f1

Remove unused RiskFactorData code

2f55b9f

TinyMarsh and others added 3 commits October 8, 2024 23:49

Increase population diversity, multiple tests

09bbeb5

Fix logic and implement feedback

c99d6cf

[pre-commit.ci] auto fixes from pre-commit.com hooks

b5c1fd5

for more information, see https://pre-commit.ci

TinyMarsh requested review from jamesturner246 and alexdewar October 9, 2024 14:16

alexdewar approved these changes Oct 11, 2024

View reviewed changes

TinyMarsh and others added 3 commits October 11, 2024 19:05

Remove commented code and set context time

ddc5198

[pre-commit.ci] auto fixes from pre-commit.com hooks

e74050d

for more information, see https://pre-commit.ci

Static cast to uint

49227a9

TinyMarsh merged commit 6c592d3 into extend_analysis Oct 14, 2024
7 checks passed

TinyMarsh deleted the calculate_index_test branch October 14, 2024 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for calculate_index function #495

Add test for calculate_index function #495

TinyMarsh commented Aug 15, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

TinyMarsh commented Sep 5, 2024

alexdewar commented Sep 5, 2024

alexdewar left a comment

alexdewar Sep 5, 2024

TinyMarsh commented Sep 5, 2024

jamesturner246 left a comment

codecov bot commented Oct 7, 2024 •

edited

Loading

TinyMarsh commented Oct 9, 2024

alexdewar left a comment

alexdewar Oct 11, 2024

TinyMarsh Oct 11, 2024

TinyMarsh Oct 11, 2024 •

edited

Loading


		size_t index_1 = analysis_module->calculate_index(test_person_1);

		ASSERT_EQ(index_1, 29);

Add test for calculate_index function #495

Add test for calculate_index function #495

Conversation

TinyMarsh commented Aug 15, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

TinyMarsh commented Sep 5, 2024

alexdewar commented Sep 5, 2024

alexdewar left a comment

Choose a reason for hiding this comment

alexdewar Sep 5, 2024

Choose a reason for hiding this comment

TinyMarsh commented Sep 5, 2024

jamesturner246 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

TinyMarsh commented Oct 9, 2024

alexdewar left a comment

Choose a reason for hiding this comment

alexdewar Oct 11, 2024

Choose a reason for hiding this comment

TinyMarsh Oct 11, 2024

Choose a reason for hiding this comment

TinyMarsh Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

TinyMarsh commented Aug 15, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

TinyMarsh Oct 11, 2024 •

edited

Loading